Skip to content

Expose /debug/pprof on operator and metrics-exporter HTTP servers#2017

Open
dentiny wants to merge 1 commit into
Altinity:0.27.2from
dentiny:hjiang/register-pprof
Open

Expose /debug/pprof on operator and metrics-exporter HTTP servers#2017
dentiny wants to merge 1 commit into
Altinity:0.27.2from
dentiny:hjiang/register-pprof

Conversation

@dentiny

@dentiny dentiny commented Jun 30, 2026

Copy link
Copy Markdown

Closes #2016

Hi team, I'd like to register pprof endpoint for profiling purpose, which is useful to understand production issues such as OOM.

Signed-off-by: dentinyhao <dentinyhao@gmail.com>
@dentiny dentiny force-pushed the hjiang/register-pprof branch from 95b7460 to 58fc138 Compare June 30, 2026 00:23
@dentiny dentiny marked this pull request as ready for review June 30, 2026 00:25
@sunsingerus sunsingerus added the planned for review This feature is planned for review label Jul 7, 2026

@sunsingerus sunsingerus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @dentiny — enabling profiling to debug OOM (#2016) is genuinely useful and something we want. I have to request changes, though, because as written this exposes /debug/pprof in a way that is a security regression for every existing install. Details and a recommended path below.

Why this can't merge as-is

import _ "net/http/pprof" runs the package init(), which registers /debug/pprof/* on http.DefaultServeMux. Both HTTP servers here serve that mux via the nil handler:

  • pkg/metrics/operator/machinery.go:110 — operator metrics, default :9999, bound 0.0.0.0 (all interfaces) unconditionally.
  • pkg/metrics/clickhouse/rest_server.go:207 — metrics-exporter, default :8888, 0.0.0.0 in the default (Plain) IPC mode.

Both ports are published cluster-wide by the clickhouse-operator-metrics Service and scraped by Prometheus, and we ship no NetworkPolicy. So after this change any pod in any namespace can reach /debug/pprof on the operator, unauthenticated:

  • Info disclosure/debug/pprof/heap and /allocs dump live operator memory, which holds ClickHouse credentials, IPC tokens, and Kubernetes API bearer tokens; /cmdline leaks all flags; /goroutine?debug=2 leaks full stacks.
  • DoS/debug/pprof/profile?seconds=N and /trace?seconds=N take an attacker-controlled duration and share the single listener/mux with /metrics, degrading the reconcile loop.
  • Bypasses IPC Secure mode — the token + loopback middleware wraps only the /chi handler; pprof is registered at the mux root, outside that protection, even in Secure mode.
  • On by default — a blank import active at init(), with no flag/config/opt-in, so every install silently gains this endpoint on upgrade.

A "just use a separate mux" fix is not sufficient

Moving pprof to a private *http.ServeMux alone does not help: net/http/pprof's init() registers on the global DefaultServeMux regardless of whether the import is blank or named, so as long as machinery.go / rest_server.go serve the nil handler they keep exposing it. (Verified: a named import still leaves DefaultServeMux responding 200 on /debug/pprof/.)

Recommended approach for a mergeable version

  1. Don't register on DefaultServeMux. Simplest is to use runtime/pprof (no init() side-effect) to serve heap/allocs/goroutine — the OOM-relevant profiles — from a private mux. Alternatively keep net/http/pprof but convert the :8888/:9999 servers to explicit, non-nil muxes so the polluted default mux is never served.
  2. Bind loopback-only (127.0.0.1), never on the metrics ports and never behind the Service — reachable via kubectl port-forward.
  3. Default OFF, opt-in via a flag, consistent with the existing -debug / -fips-info flags.
  4. Consider omitting profile/trace (the CPU/trace DoS vectors) unless separately gated, and log a warning when pprof is enabled so it is visible in kubectl logs.

Happy to review a revised version along these lines — thanks again for tackling the OOM-debuggability gap.

@sunsingerus sunsingerus added changes requested and removed planned for review This feature is planned for review labels Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants